Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download and use 16-bit SkiFree for NE tests #90

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

disinvite
Copy link
Collaborator

Resolves #3. We sort of landed on using existing binaries to test the binary image processing rather than concocting examples ourselves, at least for now.

SkiFree needs no introduction and the EXE is small. To test locally you can get the 16-bit version from Chris Pirih's site, from archive.org, or some old floppy disk you have on hand.

This fixes a bug with NE header unpacking and adds some rudimentary tests. I didn't expand the functionality yet because I'm not sure how we should handle 16-bit addresses.

We cache the download but the cache is not shared between branches so maybe using archive.org is not the best thing long-term. As we add more sample binaries we may want to parameterize the CI task and the way we load the files using pytest. Is that something we should figure out now or later?

Copy link
Collaborator

@jonschz jonschz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Only one small detail: Maybe we want to add some canonical directory to the .gitignore if one wants to keep the files there locally for testing? We could also change the CI scripts to download them there. I'd still keep the option to specify the files by path if one wants to keep them separate.

@disinvite
Copy link
Collaborator Author

Good idea! I'll look into that and see if we can add it here. It would make adding more sample binaries easier.

Should that directory just be under /tests? We could maybe try to autoload it unless you specify an alternate location at the command line.

@jonschz
Copy link
Collaborator

jonschz commented Mar 1, 2025

How about tests/binaries or something along these lines? Looking there by default sounds like a great idea, it'll simplify IDE integration. Anything will do as far as I am concerned, no need to wait for another approval.

@disinvite
Copy link
Collaborator Author

This second part is bigger than the original change so it probably needs a new review. Most of the time was spent wrangling with the CI actions trying to get the downloading and caching right.

The ultimate goal is to avoid downloading whenever possible, and that part is definitely working: each binary dependency now has its own reusable workflow file with a cache. We now also check the hash for the files just because we can.

actions/cache requires the path parameter to match between the save and restore steps or else it will report a cache miss. My solution to this was to have each sub-action download to the binfiles directory and then add that entire directory to the cache. The main binfiles action restores each the cache for each dependency and they are all merged in the binfiles directory. We then re-cache that using the key binfiles. The end result is that the unit test action only has to restore one cache, not one for each dependency, and not a list of files inside the cache. Adding new sample binaries should be simple because we just need to add a new action and then attach it to the merge action. The unit tests will use whatever is stored under binfiles.

The default location for the binary samples is tests/binfiles. If the files are there and the sha256 hash matches, we will use them in the tests. If not, the tests are skipped by default. If you add the --require-binfile option, the tests fail if we cannot load a dependency. We want this for CI, but new contributors should not see failed tests. You can provide an alternate location for the dependencies with the --binfiles parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New binary files for unit tests
2 participants